Skip to content

feat(ui): update status labels to follow consistent design pattern#2523

Merged
google-oss-prow[bot] merged 4 commits intokubeflow:mainfrom
rsun19:update-status-labels
Apr 27, 2026
Merged

feat(ui): update status labels to follow consistent design pattern#2523
google-oss-prow[bot] merged 4 commits intokubeflow:mainfrom
rsun19:update-status-labels

Conversation

@rsun19
Copy link
Copy Markdown
Contributor

@rsun19 rsun19 commented Apr 1, 2026

Description

Updates status labels across the Model Registry UI:

01-model-registry-settings 02-model-transfer-jobs 03-transfer-job-status-modal 04-catalog-source-settings 05-archived-model-details

Styling convention:

  • Interactive labels (clicking opens a popover or modal) use variant="filled"
  • Non-interactive labels use variant="outline"

Model Registry Settings (ModelRegistryTableRowStatus):

  • Renamed status labels: AvailableReady, ProgressingStarting
  • Added new Stopping status (triggered when metadata.deletionTimestamp is set)
  • Applied filled/outline variant based on interactivity

Model Transfer Jobs (ModelTransferJobTableRow / ModelTransferJobStatusModal):

  • Changed Pending label color from grey → purple
  • Changed Cancelled label text to Canceled
  • Added explicit variant="filled" on interactive table row label
  • Added variant="outline" on non-interactive modal label

Catalog Source Settings (CatalogSourceStatus / CatalogSourceStatusErrorModal):

  • Added variant="outline" to all non-interactive status labels: Connected, Failed, Starting, Unknown

Archive Details (RegisteredModelArchiveDetails, ModelVersionArchiveDetails, ArchiveModelVersionDetails):

  • Added variant="outline" to non-interactive "Archived" labels

BFF Mock Data (model_registry_settings_handler.go):

  • Added DeletionTimestamp field to Metadata struct
  • Extended createSampleModelRegistry to accept deletionTimestamp and conditions parameters
  • Added mock registries for each status (Ready, Starting, Stopping, Degrading, Unavailable) alongside existing mocks

How Has This Been Tested?

  • Added unit tests for ModelRegistryTableRowStatus: Stopping status, deletionTimestamp priority, and variant assertions (outline for non-interactive, filled for interactive)
  • Added unit tests for getStatusLabel: all 5 transfer job statuses with correct labels and colors
  • Added unit test for ModelTransferJobStatusModal: outline variant assertion
  • Created new CatalogSourceStatus.spec.tsx test suite: variant assertions for Connected, Failed, Starting, Unknown labels plus default/disabled source handling
  • Updated Cypress test assertion for CancelledCanceled
  • All 32 unit tests pass

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)

  • The commits have meaningful messages

  • Automated tests are provided as part of the PR for major new functionalities; testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).

  • The developer has manually tested the changes and verified that the changes work.

  • Code changes follow the kubeflow contribution guidelines.

  • For first time contributors: Please reach out to the Reviewers to ensure all tests are being run, ensuring the label ok-to-test has been added to the PR.

If you have UI changes

  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.
  • Verify that UI/UX changes conform the UX guidelines for Kubeflow.

@google-oss-prow google-oss-prow Bot requested review from chambridge and fege April 1, 2026 15:39
@rsun19 rsun19 changed the title updated status labels, mocks, and unit tests feat(ui): update status labels to follow RHOAI design pattern Apr 1, 2026
@rsun19 rsun19 changed the title feat(ui): update status labels to follow RHOAI design pattern [wip] feat(ui): update status labels to follow RHOAI design pattern Apr 1, 2026
@rsun19 rsun19 changed the title [wip] feat(ui): update status labels to follow RHOAI design pattern feat(ui): update status labels to follow RHOAI design pattern Apr 1, 2026
Copy link
Copy Markdown
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup overall — the filled/outline convention and label renames look good. Left a couple of inline comments: one potential issue with the shared type for deletionTimestamp, one stale test name, and a nit on test coverage.

<ModelRegistryTableRowStatus conditions={mr.status?.conditions} />
<ModelRegistryTableRowStatus
conditions={mr.status?.conditions}
deletionTimestamp={mr.metadata.deletionTimestamp}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the ModelRegistryKind type from mod-arch-shared actually include deletionTimestamp on its metadata? If not, this silently passes undefined and the "Stopping" status becomes dead code. Please verify the shared type has this field — otherwise the BFF model was updated but the frontend type contract doesn't match.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified — ModelRegistryKind is K8sResourceCommon & { metadata: { name, namespace, ... } }. The K8sResourceCommon type (from mod-arch-shared@1.12.0 in dist/types/common.d.ts) defines metadata as Partial<{ ..., deletionTimestamp: string, ... }>. TypeScript intersects both metadata shapes, so deletionTimestamp is available as an optional string. The Go BFF also includes it as *time.Time with omitempty, so the field is only present in the JSON when the resource is being deleted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(screen.getByText('Available')).toBeVisible();
expect(screen.getByText('Ready')).toBeVisible();
});
it('renders "Progressing" status', async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is titled "Progressing" but actually asserts screen.getByText('Unavailable') (line 191). Since this PR renames all the other test titles to match the new label names (Available → Ready, Progressing → Starting), this pre-existing mislabel should be fixed here too — otherwise it's the only test title that doesn't match what it verifies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed. Renamed the test title from "Progressing" to "Unavailable" to match the actual assertion.

import { ModelTransferJobStatus } from '~/app/types';
import { getStatusLabel } from '~/app/pages/modelRegistry/screens/ModelTransferJobs/ModelTransferJobTableRow';

describe('getStatusLabel', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These 5 tests mirror the switch cases 1:1 without testing edge cases (default branch, icon return value). Consider adding a test for an unknown/unexpected status value to cover the default branch, and optionally asserting icon — otherwise these are just restating the implementation as assertions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — consolidated the 5 individual tests into a single it.each table, added icon assertion (toBeDefined()), and added a new test for an unknown status value that covers the default branch (asserts raw status string, grey color, and null icon).

@@ -0,0 +1,34 @@
import { ModelTransferJobStatus } from '~/app/types';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed mostly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored — consolidated the verbose per-status tests into a compact it.each table and added a default-branch edge case test. Much leaner now.

@rsun19
Copy link
Copy Markdown
Contributor Author

rsun19 commented Apr 2, 2026

In useModelTransferJobs.spec.ts, I also fixed this warning: A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.

@rsun19 rsun19 force-pushed the update-status-labels branch from ba9f053 to 6a7b4b4 Compare April 2, 2026 19:57
@Misjohns
Copy link
Copy Markdown

Misjohns commented Apr 6, 2026

@rsun19 I was out on PTO last week. Took a look at the screen caps and have the following comments:

Model Registry UI

  • Why is the “Starting” in purple on the 6th line? The others are in gray.

Model transfer jobs

  • These labels should match the colors in the figma
    • Failed: #b1380b
    • Complete: #3d7317
    • Running: #0066cc

Model transfer jobs modal

  • Failed: Use #b1380b for both the outline and fa-exclamation-circle icon

@rsun19
Copy link
Copy Markdown
Contributor Author

rsun19 commented Apr 6, 2026

@Misjohns updated, here are the new screenshots:

Screenshot 2026-04-06 at 2 51 32 PM Screenshot 2026-04-06 at 2 51 27 PM Screenshot 2026-04-06 at 2 50 36 PM Screenshot 2026-04-06 at 2 50 27 PM Screenshot 2026-04-06 at 2 50 16 PM

@Misjohns
Copy link
Copy Markdown

Misjohns commented Apr 7, 2026

LGTM Thank you @rsun19

@rsun19 rsun19 requested a review from manaswinidas April 9, 2026 13:03
@rsun19
Copy link
Copy Markdown
Contributor Author

rsun19 commented Apr 9, 2026

@rsun19 rsun19 changed the title feat(ui): update status labels to follow RHOAI design pattern feat(ui): update status labels to follow consistent design pattern Apr 9, 2026
@rsun19
Copy link
Copy Markdown
Contributor Author

rsun19 commented Apr 9, 2026

/hold

@rsun19
Copy link
Copy Markdown
Contributor Author

rsun19 commented Apr 9, 2026

/unhold

@rsun19
Copy link
Copy Markdown
Contributor Author

rsun19 commented Apr 9, 2026

/hold

@christianvogt
Copy link
Copy Markdown
Contributor

We need to validate the material UI components function with these changes and if we need any theme fixes.
@jenny-s51 can help with guiding theme changes if necessary

@rsun19
Copy link
Copy Markdown
Contributor Author

rsun19 commented Apr 16, 2026

Blocked by #2598

@manaswinidas
Copy link
Copy Markdown
Contributor

manaswinidas commented Apr 21, 2026

#2598 is merged

@lucferbux
Copy link
Copy Markdown
Contributor

@jenny-s51 are those changes validated, I've run it after rebasing with main and everything looks fine, but I don't have full context, can you take a look and if everything looks great we can lgtm and approve

rsun19 added 3 commits April 27, 2026 09:29
Signed-off-by: rsun19 <robertssun1234@gmail.com>
Signed-off-by: rsun19 <robertssun1234@gmail.com>
Signed-off-by: rsun19 <robertssun1234@gmail.com>
@rsun19 rsun19 force-pushed the update-status-labels branch from 0b6e51b to 99c0fee Compare April 27, 2026 13:30
@rsun19
Copy link
Copy Markdown
Contributor Author

rsun19 commented Apr 27, 2026

/unhold

Signed-off-by: rsun19 <robertssun1234@gmail.com>
@rsun19
Copy link
Copy Markdown
Contributor Author

rsun19 commented Apr 27, 2026

Screenshot 2026-04-27 at 9 42 13 AM

Copy link
Copy Markdown
Member

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rsun19 - LGTM, verified in the MR hub UI - labels look and work as expected with the bump to v1.15.4 of the mod arch library packages. 🎉

This PR is ready to approve at your convenience @lucferbux - thank you 🙏

@manaswinidas
Copy link
Copy Markdown
Contributor

/approve

@google-oss-prow
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: manaswinidas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow Bot merged commit d6549fe into kubeflow:main Apr 27, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants